Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'nginx_manage_repo' feature flag and defaults #420

Merged
merged 3 commits into from
Jun 14, 2021

Conversation

sjugge
Copy link
Contributor

@sjugge sjugge commented Jun 11, 2021

Proposed changes

This change adds a nginx_manage_repo boolean which can be used to disable Nginx repo management by this role.

This feature flag addresses edge-cases where Nginx' repositories are managed by other means and should not be configured by this role in order to install Nginx. The default behavior of the role is unaltered.

Checklist

  • I have read the CONTRIBUTING document
  • I have added Molecule tests that prove my fix is effective or that my feature works
  • I have checked that any relevant Molecule tests pass after adding my changes
  • I have updated any relevant documentation (defaults/main/*.yml, README.md and CHANGELOG.md)

Sidenote on Molecule tests, as a test for setting nginx_manage_repo to false would effectively require current tests to be disabled, and as the default behavior is not altered, no additional tests have been added.

@alessfg alessfg self-requested a review June 14, 2021 11:10
Copy link
Collaborator

@alessfg alessfg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpick, can you rename Nginx to NGINX in comments such as #420 (files)? Can you update the Changelog too?

Re Molecule tests, you could arguably create a new scenario and setup the NGINX repo beforehand in the prepare.yml playbook (you can check an example under the plus molecule tests and you can probably copy/paste the same tasks found in the role proper to setup the NGINX repository in the prepare.yml playbook). Not necessarily a requirement to merge this PR, but it would be nice.

- update CHANGELOG
- s/Nginx/NGINX/g
@sjugge
Copy link
Contributor Author

sjugge commented Jun 14, 2021

@aknot242 just updated the CHANGELOG and properly named NGINX.

As for the test, if we can move forward with this PR as -is that'd be great as it would unblock us, I'm under a bit of a crunch ATM but when I have the time I'll re-visit the proposed approach. Using the prepare.yml is a good suggestion.

@alessfg alessfg changed the title add 'nginx_manage_repo' feature flag and defaults Add 'nginx_manage_repo' feature flag and defaults Jun 14, 2021
@alessfg alessfg merged commit 3a63b9a into nginxinc:main Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants